Add AsyncFacilitator to enable an async, event-driven, non-polling me… #29364
+1,119
−474
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
9ebcf9c
to
73c6d9a
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov,
lpbeliveau-silabs and
LuDuda
…vent driven approach rather than polling - Update the OTA provider darwin implementation to use the AsyncTransferFacilitator to transfer the OTA file using BDX
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
277f287
to
db59df6
Compare
|
PR #29364: Size comparison from b6a1304 to db59df6 Full report (22 builds for cc13x4_26x4, cc32xx, nrfconnect, nxp, qpg, stm32, tizen)
|
nivi-apple
commented
Sep 24, 2024
| return CHIP_NO_ERROR; | ||
| } | ||
| } | ||
| return CHIP_ERROR_INCORRECT_STATE; |
There was a problem hiding this comment.
Returning CHIP_ERROR_INCORRECT_STATE. I looked at the spec and there is Unexpected Message status code that should be sent for any BDX messages but we have not yet established a session yet. the unexpected message does map to CHIP_ERROR_INCORRECT_STATE so thats what I used here.
|
PR #29364: Size comparison from b6a1304 to d344a19 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #29364: Size comparison from 4a21a8d to 4eb2b86 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #29364: Size comparison from 4a21a8d to afb50a5 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| * output events generated by the TransferSession state machine until it receives an output event of type | ||
| * TransferSession::OutputEventType::kNone. For each output event it receives, it either sends a message accross the exchange if the | ||
| * TransferSession generated an event of type TransferSession::OutputEventType::kMsgToSend or calls the HandleTransferSessionOutput | ||
| * method to notify the subclass of the output event which has the business logic of handling the BDX message and calling the |
There was a problem hiding this comment.
This is a badly run-on sentence....
More generally: seems like this comment should describe how/when this class should be used, not its implementation details. Please let me know if you need help with that.
|
|
||
| void ProcessOutputEvents(); | ||
|
|
||
| void CleanUp(); |
There was a problem hiding this comment.
This needs some really clear documentation on who is expected to call it and when. When is it safe to call? Is it safe to call synchronously from under HandleTransferSessionOutput? Is it required to call it at various points?
The PR comments above say:
It's called by subclass for various scenarios including error conditions
but wouldn't error conditions call NotifyEventHandled with an error? This part really needs to be very clearly defined so we can get the invariants right.
| * @param[in] event The OutputEvent that was handled by the subclass. | ||
| * @param[in] status The error code that occured when handling the event if an error occurs. Otherwise CHIP_NO_ERROR. | ||
| */ | ||
| void NotifyEventHandled(TransferSession::OutputEvent & event, CHIP_ERROR status); |
There was a problem hiding this comment.
This should be const, and ToString on OutputEvent should be a static method, since it does not depend on any state of the OutputEvent itself.
Co-authored-by: Boris Zbarsky <[email protected]>
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
27e25d2
to
01f12ac
Compare
nivi-apple
commented
Oct 24, 2024
| assertChipStackLockedByCurrentThread(); | ||
|
|
||
| // TODO: #36181 - Store the imageTransferHandler in a set of MTROTAImageTransferHandler objects. | ||
| mOTAImageTransferHandler = static_cast<MTROTAImageTransferHandler *>(imageTransferHandler); |
There was a problem hiding this comment.
@bzbarsky-apple should i re-assign this here or just verify that it matches. if it doesn't clean up everything by calling destroyself?
|
PR #29364: Size comparison from 1febc9b to 4029261 Full report (3 builds for cc32xx, stm32)
|
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
c5a62a6
to
d3713d7
Compare
|
PR #29364: Size comparison from 1febc9b to d3713d7 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…chanism for facilitating BDX transfers
Currently BDX transfers can only use the TransferFacilitator which polls the BDXTransferSession with a poll interval that adds unneccessary delays and is not the ideal approach to handle BDX transfers
Add support for an AsyncTrasferFacilitator that uses a non-polling, event-driven mechanism based on BDX messages received over the exchange context and gets the next output event from the transfer session to drive the BDX transfer
Add support for an AsyncResponder that uses the AsyncTrasferFacilitator and enables a provider delegate to handle BDX transfers
Modify the darwin OTA provider code to use the AsyncResponder
Add support for handling multiple OTA requests to the ota provider by creating an MTROTAImageTransferHandler object that handles a BDX transfer session independently from a singleton unsolicited BDX message handler that is created by the MTROTAProviderDelegateBridge.
Support only one BDX transfer session and return busy to any query images coming from other nodes for now to match the test expectations (to be removed once tests for multiple OTA are in place)